fix(service-automation): scope a `map` node's progress state to one execution of its collection by os-warren · Pull Request #15648 · objectstack-ai/objectstack

os-warren · 2026-09-05T01:39:54Z

A map node inside a loop body ran its collection on the first iteration only. Iterations 2..n ran nothing, the map step reported success on every one of them, and the run finished completed.

Mechanism

map tracks its progress through the collection in the flow variable nodeId.$mapState, and wrote it into the flow's shared variable scope without ever removing it. A loop body region runs in that same scope by construction — runRegion is handed the caller's Map deliberately, because the iterator variable and the body's mutations have to stay visible to the rest of the flow. So the state written by iteration 1 was still there when iteration 2 entered the map: it read back started === collection.length, correctly concluded there was nothing left to start, and returned.

The lifetime was the defect, not the key. $mapState was scoped to the run; it should be scoped to one execution of the collection.

The fix

One line of behaviour: the terminal path now deletes the state key instead of re-setting it.

The durable-pause path is deliberately untouched, and that is the load-bearing half of this PR. The card's own scope note warned that "delete the state key when the collection is exhausted" was the obvious candidate and was NOT MEASURED, because a map resumed mid-collection depends on that state surviving. That is established here rather than assumed:

map writes $mapState in two places — the suspend arm (map-node.ts, before returning suspend: true) and the terminal arm. Only the terminal one changes.
On suspend the engine snapshots the whole scope (Object.fromEntries(variables)) into the SuspendedRun. On resume, resumeInternal rebuilds the scope with new Map(Object.entries(run.variables)) — a fresh map from that snapshot. So the suspend-time write is the only write to this key a resume can ever read, and no terminal-path change can reach it.
The one reader outside map-node.ts (resume-authority-gate.test.ts) reads the key off a suspended run's snapshot, so it is on the same side of that line.

A pausing map is in any case unreachable from inside a loop body: runRegion converts a durable pause inside a structured region into an error. The resume path is live for a top-level map, which is what the new resume pin exercises.

Tests

New file map-in-loop-iteration-state.test.ts — four pins on the real AutomationEngine, no mock of the mechanism:

the card's measurement — 5 iterations x 2 items produces 10 child runs (it produced 2);
the run is completed with summary.failed === 0 and the work happened — pinning why failed could never have been the instrument that caught this;
each iteration collects a fresh result set, and nodeId.$mapState is absent from the scope after the map completes (the mechanism asserted directly, not inferred from the child-run count);
the resume half: $mapState is present in the suspend snapshot with started: 1, and driving the pauses through runs every item exactly once.

Both pins were mutated to confirm they discriminate, restoring from HEAD each time and verifying the restore by git diff HEAD plus a blob-hash comparison:

mutation	result
terminal `delete` reverted to `set` (the pre-fix spelling)	all 4 pins red
unconditional delete — the suspend arm deletes too, i.e. the naive fix the card warned about	the three loop pins stay green; only the resume pin fails

That second row is the point. The obvious fix makes the reported symptom go away and silently breaks resume, and the resume pin is the only thing standing between the two.

Census — the second deliverable

Searched for the predicate (executors that persist state under a node-scoped key in the shared scope) rather than recalling which ones do. Every variables.set / variables.delete in every builtin, every key expression derived from node.id, module-scope mutable state, writes into the shared context, and every registerNodeExecutor in the repo — including the two in plugin-approvals, which are outside this package.

map is the only one. Every other scope write is an author-named output variable (crud, screen, subflow, logic, try_catch's errorVariable) or a loop's iterator/index binding — written each time, never read back as progress. wait and the approval nodes only read the scope; parallel never touches it. The zero has a firing control: the same patterns hit map's seven writes, including the defective one, and hit wait's and the approval nodes' reads.

⇒ No second instance of the identical mechanism, so nothing beyond map is changed here.

Found and NOT fixed

service-automation: a PAUSING map inside a contained region leaves its progress state behind — later loop iterations skip items and the exhausted map returns success having run nothing #15646 — the same key reached through the other arm. When a pausing map sits inside a contained region, the suspend write lands but the pause is refused by runRegion, leaving residue no terminal path can clear. Measured: 3 iterations x 2 items, 0 items ever completed, only 2 of 3 iterations reached the catch, and iteration 3 returned success having run nothing. Closing it needs a ruling on which layer owns cleanup when a suspend is refused, so it is filed rather than guessed.
spec: FlowRunSummary's two paragraphs disagree for a subflow parent — failed is declared a node fold, while the summary is declared to answer "what did this run cause" and roll a child's totals up #15617 is not addressed here; that run's summary.failed = 0 over two contained failures is recorded on service-automation: a PAUSING map inside a contained region leaves its progress state behind — later loop iterations skip items and the exhausted map returns success having run nothing #15646 as a data point for it.

Gates

pnpm lint (full repo) exit 0; package test 108 files / 1294 tests and typecheck both exit 0, re-run at the final commit ac3079c43. ADR-0087 via the real invocation node scripts/check-adr-0087-registration.mjs --base origin/main --head ac3079c43 exit 0 (1 non-breaking changeset(s) seen), plus its --self-test. The gate set was re-derived from the actual changed files with scripts/pm/dispatch-gates.mjs; the derived families were run and each zero was read off the gate's own verdict line with the exit code captured by redirect, never through a pipe.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y

Generated by Claude Code

…llection Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y

…xecution of its collection `map` tracks progress through its collection in the flow variable `<nodeId>.$mapState` and wrote it into the flow's SHARED variable scope without ever removing it. A `loop` body region runs in that same scope by construction, so the state written by iteration 1 was still there when iteration 2 entered the map: it read back `started === collection.length`, concluded there was nothing left to start, and returned success. Measured on the real engine: 5 iterations x 2 items produced 2 child runs instead of 10, the map step reported `success` on all five iterations, and the run finished `completed` with `failed = 0` — silent partial work, invisible to the very run-level counter built to expose that class. The state key is now removed once the collection is exhausted, making its lifetime one execution of the collection rather than the enclosing scope's. The durable-pause path is deliberately untouched: the write made before returning `suspend: true` is the mechanism a resume depends on, because `resumeInternal` rebuilds the scope from the snapshot taken at that suspend and can never see a later write. Only the terminal path clears the key. A test pins that half — an unconditional delete leaves the loop assertions green and fails only the resume pin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y

github-actions · 2026-09-05T01:40:33Z

📓 Docs Drift Check

1 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see

the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 1be26b0de27b2c45d208a20edffefbac1e4892c2 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 826e703f56407a720d02b8ed03f1bf257a0e4488 — the merge of head ac3079c437e48e2b8bc5aa57286a31454b739e67 into base 1be26b0de27b2c45d208a20edffefbac1e4892c2, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 826e703f56407a720d02b8ed03f1bf257a0e4488 && git checkout 826e703f56407a720d02b8ed03f1bf257a0e4488
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 1be26b0de27b2c45d208a20edffefbac1e4892c2 ac3079c437e48e2b8bc5aa57286a31454b739e67 && git checkout -B drift-repro 1be26b0de27b2c45d208a20edffefbac1e4892c2 && git merge --no-ff ac3079c437e48e2b8bc5aa57286a31454b739e67

node scripts/docs-audit/affected-docs.mjs --json 1be26b0de27b2c45d208a20edffefbac1e4892c2

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

os-warren · 2026-09-05T01:59:45Z

Clause-② contract review — PR #15648 (card #15616) · PASS

Tier line. CONTRACT_REVIEW_TIER = claude-fable-5-1 (scripts/pm/dispatch-gates.mjs:8659). This review's serving-model evidence is (a) the dispatching PM's attestation that the Agent call carried an explicit model: fable override, and (b) this reviewer's own system-prompt identity, claude-fable-5-1. No session-level field corroborates a per-subagent override — get_session describes the parent session, which reports claude-opus-5 after a user switch — so this is stated as "override + self-report", not as a match against an auditable field. (The first pass of this review stopped at the fuse for exactly that reason; the PM corrected the fuse's source of truth.)

Reviewed at head ac3079c43 (merge-base d30ccb9bd), in a detached worktree; no push, no branch touched. Head sha / file count / blob 0f4c526d0 / ADR-0087 gate were already verified by the PM and are not re-done here.

What was driven (not read)

Single pin file via the package's own vitest (the PM already re-certified the 108-file package union; this round is discrimination only). Restores by git checkout HEAD -- then git hash-object compared to 0f4c526d077cbc3d8e369e22a8ec0a71ba06949f; an empty hash was wired as FAILURE. All three restores matched; git diff HEAD empty after each.

run	pin 1 (10 child runs)	pin 2 (`failed=0` + 10)	pin 3 (fresh results, key absent)	pin 4 (resume)
head, unmutated	✓	✓	✓	✓
A — terminal `delete` → `set` (the pre-fix spelling)	✗ `[ 'r1:a', 'r1:b' ]` ≠ 10	✗ length 2	✗ results re-read + key present	✗
B — unconditional delete: suspend arm `set` → `delete` (the naive fix)	✓	✓	✓	✗ only — `expected undefined to match { started: 1 }`
C (mine) — `delete` on ENTRY before the read, terminal delete kept	✓	✓	✓	✗ only — `expected [ 'a', 'a', 'a' ] to deeply equal [ 'a', 'b', 'c' ]`

⭐ The mutation table's second row reproduces exactly: B leaves the three loop pins green and fails only the resume pin. Two independent naive spellings (B, C) are both caught by pin 4 alone, and A shows all four pins discriminate against the unfixed source. Under B the pin dies at its snapshot assertion before reaching the "every item once" assertion, so I also ran my own probes under B: the nested map-in-map probe shows the predicted re-run signature directly — [ 'o1:i1', 'o1:i1', 'o1:i1', 'o1:i1' ] — the resumed map restarting at item 0 on every resume, which is the sentence the new docblock (map-node.ts:236) makes.

Five additional probes of my own, all green at head (file lived only in my worktree, deleted after):

P1 empty collection inside a loop — 3 iterations × []: each sees out = [], m.count = 0, no $mapState, run success (attack item 5).
P2 nested map → child flow with its own map → pausing grandchild — 2 × 2, four resumes: every leaf exactly once, three suspensions unwind to zero, no $mapState residue in the inner or outer scope (attack item 1, the "nested resume" case).
P3 mixed [sync, pause, sync] in one top-level map — snapshot holds started: 2; the single resume runs the trailing sync item inline, terminal path clears the key, after sees the three results.
P4 item failure contained by try_catch inside a loop — the entry that is neither suspend nor terminal: no set on the failure path, so iteration 2 re-runs item 0; no residue in any of the three catches.
P5 service-automation: a PAUSING map inside a contained region leaves its progress state behind — later loop iterations skip items and the exhausted map returns success having run nothing #15646 reproduction — loop › try_catch › map with a pausing child, 3 × 2: ran = [], two catches, summary.failed = 0, run success — the follow-up's mechanism is real and this PR correctly does not touch it.

The eight attack items

Resume-path argument — every link holds, by reading and by driving. Exactly two writes to stateKey in map-node.ts (set :187 in the suspend arm, delete :238 terminal; the handoff keys at :137–138 are the siblings, not this key). All three suspend-snapshot sites (engine.ts:4387, :5478, :8722) are Object.fromEntries(variables). resumeInternal rebuilds a fresh map — new Map<string, unknown>(Object.entries(run.variables)) at :5298 — and a map: correlation re-enters via executeNode (:5378) with the engine-built $mapItemDone/$mapItemOutput signal folded by applyResumeSignal. The other new Map(Object.entries(run.variables)) (:5633) is a read-only visibility scope for screen-input refusal, not a resume. Bubble (bubbleToParent :5728) goes through the same resumeInternal, so a child completion can only ever read the parent's suspend-time snapshot. No entry path reaches the terminal delete before a read that needs the state: the only reader is the node itself at entry (:128), and the only entries that find the key present are post-resume entries reading the snapshot. ⭐ One link the dev did not claim, worth its own line: applyResumeSignal (:1103) rejects any non-engine-built signal that names an engine variable ($-prefixed or containing .$), so $mapState cannot be forged or clobbered from outside — the suspend-arm write is the only write a resume can read and the only write that can exist.
Second mutation row — reproduced, see table; plus an independent spelling (C) and the re-run signature under B.
All four pins discriminate — mutation A turns all four red for the right reasons (2 child runs; length 2; iteration-1 results re-read and key present; key present after completion).
Census zero — re-derived independently, survives. Predicate: any executor persisting state under a node-scoped key in the shared scope and reading it back on a later entry. Swept every variables.set/delete/get/has in all 15 executor files (13 builtins + plugin-approvals' two), every ${node.id}.-derived key in non-test source, module-scope mutable state, and registerNodeExecutor( repo-wide including apps/, scripts/, and the objectui checkout. Result: map is the only one. The only other ${node.id}. writer is the engine's own output merge (engine.ts:7571), which nothing reads back as progress — the dev's "observed but distinct" note is correct. Legacy $loopItems/$loopIndex are written and never read. try_catch compares $error by identity before/after (innerError !== errorBefore), so a stale $error from a prior iteration cannot count. wait and the approval executors read only $runId/$record/$flowName/$flowLabel; parallel, notify, http, connector, approval-revise touch the scope not at all. Outside packages/, registerNodeExecutor( appears only in two lint scripts. The dev's control ("the same patterns fire on map's seven writes") is consistent with the count I get (4 set + 3 delete post-fix).
Ordering and collateral — state.results is the in-memory array, unaffected by deleting the key; the engine then writes <id>.results/<id>.count from result.output as before. Nothing downstream reads $mapState: repo-wide readers are map-node.ts, resume-authority-gate.test.ts (off a suspended run's variables, :400–401, same side of the line as the dev says), comments in engine.ts, ADR-0019, and CHANGELOGs. Empty collection driven (P1).
runRegion — confirmed at engine.ts:7867–7871: isSuspendSignal(err) → throw new Error('durable pause inside a structured region … is not supported'). Driven by P5. This is both what makes the terminal-only delete complete for the loop case and the mechanism behind service-automation: a PAUSING map inside a contained region leaves its progress state behind — later loop iterations skip items and the exhausted map returns success having run nothing #15646.
Honesty audit. (a) check:type-check-debt --re-measure NOT MEASURED is an honest limit: the gate's own header says --re-measure "refreshes the ledgered packages' dependency closure, then runs tsc per ledger entry", and an unbuilt closure is exit 3 — prerequisite not met, "the run says NOTHING about the ledger"; the dev did not claim a pass. (b) Claim: the claim comment exists (2026-09-05T01:03:24Z, session 03324ae2…, branch named, base named, comments re-read before claiming); assignees are empty and the dev says so. ⚠️ The repo's own CLAUDE.md says "assign yourself and comment"; the dev's "assignee is the PM's field" reading is a deviation from that text, self-reported rather than silently resolved. PM's to rule, not a blocker for this PR. (c) MCP-call arithmetic: the published os-dev-report lists eight calls and claims 8 — internally consistent on GitHub. Per the PM, the dev's off-channel report gives the true total as 9 with the reason; the published figure is then an undercount by one, disclosed rather than hidden. I cannot see the private report and do not re-verify the 9.
service-automation: a PAUSING map inside a contained region leaves its progress state behind — later loop iterations skip items and the exhausted map returns success having run nothing #15646 — exists, filed 01:36:08Z by the dev; describes the suspend-arm residue (write lands, runRegion refuses, no terminal path clears); carries pm:queue only, no domain:* / type / priority, no assignees; body states those are triage's. Mechanism reproduced (P5). One note on its measured block: its printed stateSeen = [ {started: 2}, {started: 2} ] is an aliasing artefact — both entries are the same object mutated in place at iteration 2. Cloned at catch time it reads [ {started: 1}, {started: 2} ], which is what the card's own iteration-by-iteration narrative says. Cosmetic; the narrative is right.

Notes (non-blocking)

The docblock/PR/changeset quote new Map(Object.entries(run.variables)) drops the <string, unknown> type parameter that the source carries. Immaterial to the argument.
The changeset is patch for @objectstack/service-automation with the pause-path invariant stated — proportionate to a one-line lifetime fix.
Observed, not driven, pre-existing, outside this diff: Object.fromEntries(variables) is a shallow snapshot and the in-memory store keeps object identity, so a resumed map mutating state in place also mutates the parked snapshot's object. It would only matter on the consumed-suspension restore path ([Decision] Workflow resume ordering: a thrown node today leaves the run terminally unresumable — which of three shapes, given that the current order buys exactly-once across a crash? #13937 "stranded") with the in-memory store; a serialising durable store is immune. Reported for the PM to decide whether it is worth a card; not a finding against this PR.
One-line wording nit: "Both pins were mutated" in the PR body means the two halves (loop pins vs. resume pin); there are four pins. Not misleading in context.

NOT MEASURED (by this review)

The full package suite / typecheck / pnpm lint at this head — the PM's re-run is relied on; I ran only the pin file and my probes.
The dev's tsc --listFiles coverage claim for the new test file.
check:type-check-debt --re-measure (needs the built closure; CI owns it).
CI check-run status on the head.

Verdict: PASS. One behavioural line; the resume argument holds on every link and under driving; the naive fix is caught by the resume pin alone in two spellings; the census zero re-derives independently; the follow-up is filed correctly. Landing is the PM's.

Generated by Claude Code

claude added 2 commits September 5, 2026 01:20

wip(service-automation): scope $mapState to one execution of the co…

b119d0e

…llection Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y

github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Sep 5, 2026

os-warren marked this pull request as ready for review September 5, 2026 02:01

os-warren enabled auto-merge September 5, 2026 02:01

os-warren added this pull request to the merge queue Sep 5, 2026

os-warren mentioned this pull request Sep 5, 2026

Two governed surfaces contradict each other on who sets the issue assignee: CLAUDE.md says the claiming agent does, os-dev.md rule 2 says the dev seat never does #15670

Closed

Merged via the queue into main with commit 7bf96cf Sep 5, 2026
35 checks passed

os-warren deleted the claude/issue-15616-map-loop-iteration-state branch September 5, 2026 02:43

os-warren mentioned this pull request Sep 5, 2026

service-automation: a map node inside a loop body runs its collection ONCE — iterations 2..n do nothing, report success, and the run completes green #15616

Closed

Navigation Menu

Uh oh!

fix(service-automation): scope a `map` node's progress state to one execution of its collection - #15648

fix(service-automation): scope a `map` node's progress state to one execution of its collection#15648
os-warren merged 2 commits into
mainfrom
claude/issue-15616-map-loop-iteration-state

os-warren commented Sep 5, 2026

Uh oh!

github-actions Bot commented Sep 5, 2026

Uh oh!

os-warren commented Sep 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

os-warren commented Sep 5, 2026

Mechanism

The fix

Tests

Census — the second deliverable

Found and NOT fixed

Gates

Uh oh!

github-actions Bot commented Sep 5, 2026

📓 Docs Drift Check

Uh oh!

os-warren commented Sep 5, 2026

Clause-② contract review — PR #15648 (card #15616) · PASS

What was driven (not read)

The eight attack items

Notes (non-blocking)

NOT MEASURED (by this review)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants